Fix System.OverflowException in CustomScrollViewerEx during Mouse Wheel scroll#4497
Fix System.OverflowException in CustomScrollViewerEx during Mouse Wheel scroll#44974yinn wants to merge 6 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an early-return guard in CustomScrollViewerEx.OnMouseWheel when ActualHeight <= 0, minor non-functional formatting in the control, a ProjectReference from the test project to the main project, and a new NUnit STA test that invokes the non-public OnMouseWheel via reflection and asserts it does not throw. ChangesMouseWheel Handler Robustness
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs (1)
91-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset animation state on guard-path returns.
SetIsAnimating(this, true)is set before guard checks, but both guard returns exit without resetting it. That can leave animation state stuck astrueand suppress offset state updates inOnScrollChanged.Suggested fix
- ScrollViewerBehavior.SetIsAnimating(this, true); - if (Direction == Orientation.Vertical) { + if (ActualHeight <= 0) + return; + + ScrollViewerBehavior.SetIsAnimating(this, true); + if (ScrollableHeight > 0) { e.Handled = true; } - - if (ActualHeight <= 0) - return;else { + if (ActualWidth <= 0) + return; + + ScrollViewerBehavior.SetIsAnimating(this, true); + if (ScrollableWidth > 0) { e.Handled = true; } - - if (ActualHeight <= 0) - return;Also applies to: 133-134
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs` around lines 91 - 101, The handler sets ScrollViewerBehavior.SetIsAnimating(this, true) before guard checks but returns on two guard paths without resetting it; update the method in CustomScrollViewerEx (the scroll event handler that calls SetIsAnimating) so that you either move the SetIsAnimating(this, true) call to after the guard checks or ensure you call ScrollViewerBehavior.SetIsAnimating(this, false) before each early return (including the guard near the Vertical Orientation block and the other guard around lines ~133-134) so the animation flag is never left true when exiting early.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Flow.Launcher.Test/MouseWheelTest.cs`:
- Around line 26-34: The reflection lookup for CustomScrollViewerEx's
"OnMouseWheel" is not asserted before Invoke, which causes a confusing
null-reference if the method isn't found; add an explicit null assertion (e.g.
Assert.IsNotNull or Assert.NotNull) for onMouseWheelMethod after the GetMethod
call with a clear message like "OnMouseWheel method not found on
CustomScrollViewerEx" so failures are diagnostic, then proceed to call
onMouseWheelMethod.Invoke(scrollView, new object[] { e }) inside the existing
Assert.DoesNotThrow block.
In `@Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs`:
- Around line 133-136: In CustomScrollViewerEx (the mouse wheel/scroll handling
code that computes WheelChange using e.Delta * (ViewportWidth / 1.5) /
ActualWidth), change the early-return guard to validate ActualWidth instead of
ActualHeight (i.e., if (ActualWidth <= 0) return;) so you never divide by zero
when computing WheelChange; update the check in the same method where
WheelChange is declared to prevent the infinite offset on horizontal scrolling.
---
Outside diff comments:
In `@Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs`:
- Around line 91-101: The handler sets ScrollViewerBehavior.SetIsAnimating(this,
true) before guard checks but returns on two guard paths without resetting it;
update the method in CustomScrollViewerEx (the scroll event handler that calls
SetIsAnimating) so that you either move the SetIsAnimating(this, true) call to
after the guard checks or ensure you call
ScrollViewerBehavior.SetIsAnimating(this, false) before each early return
(including the guard near the Vertical Orientation block and the other guard
around lines ~133-134) so the animation flag is never left true when exiting
early.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e62e2923-df1f-435a-97a3-52643a176be2
📒 Files selected for processing (3)
Flow.Launcher.Test/Flow.Launcher.Test.csprojFlow.Launcher.Test/MouseWheelTest.csFlow.Launcher/Resources/Controls/CustomScrollViewerEx.cs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs`:
- Around line 133-134: In OnMouseWheel of CustomScrollViewerEx the method sets
the animating flag (IsAnimating) earlier but returns early when ActualWidth <= 0
without clearing it, which can leave IsAnimating stuck; modify OnMouseWheel to
reset IsAnimating (or the animating field) to false before the early return so
the animation state is cleared and OnScrollChanged can continue syncing offsets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea371989-0f85-4582-a0d5-ae6ec2cb5aef
📒 Files selected for processing (1)
Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs (1)
100-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVertical guard must also reset animation state.
The horizontal path (lines 133-137) correctly resets
IsAnimatingtofalsebefore returning, but the vertical path here does not. SinceSetIsAnimating(true)is called at line 91, this early return will leaveIsAnimatingstuck, preventingOnScrollChanged(line 168) from syncing offsets.Proposed fix
- if (ActualHeight <= 0) - return; + if (ActualHeight <= 0) + { + ScrollViewerBehavior.SetIsAnimating(this, false); + return; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs` around lines 100 - 101, The vertical early-return when ActualHeight <= 0 fails to reset the animation flag and leaves IsAnimating true; update the vertical guard in the same method that calls SetIsAnimating(true) to call SetIsAnimating(false) (or otherwise reset IsAnimating) before returning so the animation state is cleared and OnScrollChanged can resync offsets; mirror the horizontal path's behavior that resets IsAnimating to false prior to returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs`:
- Around line 100-101: The vertical early-return when ActualHeight <= 0 fails to
reset the animation flag and leaves IsAnimating true; update the vertical guard
in the same method that calls SetIsAnimating(true) to call SetIsAnimating(false)
(or otherwise reset IsAnimating) before returning so the animation state is
cleared and OnScrollChanged can resync offsets; mirror the horizontal path's
behavior that resets IsAnimating to false prior to returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ed14a09-ed58-4711-9720-2e30a96598f7
📒 Files selected for processing (1)
Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs (1)
90-91:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDirection-agnostic guard reintroduces horizontal divide-by-zero path.
The new guard checks only height, but horizontal scrolling still divides by
ActualWidth(Line 133). IfActualWidth == 0,WheelChangecan become infinite again and propagate into animation timing.Suggested minimal fix
protected override void OnMouseWheel(MouseWheelEventArgs e) { - if (ActualHeight <= 0) - return; - - var Direction = GetDirection(); + var Direction = GetDirection(); + if ((Direction == Orientation.Vertical && ActualHeight <= 0) || + (Direction == Orientation.Horizontal && ActualWidth <= 0)) + { + return; + } + ScrollViewerBehavior.SetIsAnimating(this, true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs` around lines 90 - 91, The early return only checks ActualHeight but the code later computes WheelChange by dividing by ActualWidth (risking divide-by-zero); update the guard in CustomScrollViewerEx (the method containing the WheelChange calculation) to check both ActualHeight and ActualWidth (return if either <= 0) or otherwise protect the WheelChange computation by ensuring ActualWidth is non-zero before dividing; reference ActualWidth, ActualHeight and WheelChange to locate and fix the division-by-zero path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs`:
- Around line 90-91: The early return only checks ActualHeight but the code
later computes WheelChange by dividing by ActualWidth (risking divide-by-zero);
update the guard in CustomScrollViewerEx (the method containing the WheelChange
calculation) to check both ActualHeight and ActualWidth (return if either <= 0)
or otherwise protect the WheelChange computation by ensuring ActualWidth is
non-zero before dividing; reference ActualWidth, ActualHeight and WheelChange to
locate and fix the division-by-zero path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 326c3eca-b827-4876-857e-e9b013a3b57e
📒 Files selected for processing (1)
Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs
Jack251970
left a comment
Jack251970
left a comment
There was a problem hiding this comment.
Thanks for your contribution! Let us wait for users' feedbacks!
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs (1)
88-89:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard the dimension for the resolved scroll direction.
Line 88 checks
ActualHeightbeforeGetDirection(), but Line 131 divides byActualWidth. If horizontal scrolling is selected (Orientation.Horizontalor Shift-modified input),ActualWidth == 0still reaches the horizontal division and can reproduce the same overflow path this fix is meant to close.Suggested fix
protected override void OnMouseWheel(MouseWheelEventArgs e) { - if (ActualHeight <= 0) - return; - var Direction = GetDirection(); + if ((Direction == Orientation.Vertical && ActualHeight <= 0) || + (Direction == Orientation.Horizontal && ActualWidth <= 0)) + { + return; + } + ScrollViewerBehavior.SetIsAnimating(this, true);Also applies to: 91-92, 124-131
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs` around lines 88 - 89, The guard currently checks ActualHeight before calling GetDirection(), but when scrolling is horizontal the code later divides by ActualWidth (in methods like the measure/arrange logic inside CustomScrollViewerEx and any calls to GetDirection()), so add a direction-aware guard: call GetDirection() first, then if direction is Vertical ensure ActualHeight > 0, and if Horizontal ensure ActualWidth > 0 (also apply the same pattern to the other similar blocks referenced around lines 91-92 and 124-131). Update the early-return checks so they depend on the resolved Orientation from GetDirection() to prevent zero-dimension division for the selected scroll direction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs`:
- Around line 88-89: The guard currently checks ActualHeight before calling
GetDirection(), but when scrolling is horizontal the code later divides by
ActualWidth (in methods like the measure/arrange logic inside
CustomScrollViewerEx and any calls to GetDirection()), so add a direction-aware
guard: call GetDirection() first, then if direction is Vertical ensure
ActualHeight > 0, and if Horizontal ensure ActualWidth > 0 (also apply the same
pattern to the other similar blocks referenced around lines 91-92 and 124-131).
Update the early-return checks so they depend on the resolved Orientation from
GetDirection() to prevent zero-dimension division for the selected scroll
direction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b2ded99-0ef6-43c7-995b-a733a5341c21
📒 Files selected for processing (1)
Flow.Launcher/Resources/Controls/CustomScrollViewerEx.cs
2 participants
This PR fixes the OverflowException reported in #4481, which occurs when the user scrolls on a ScrollViewer that has not been fully rendered yet or whose ActualHeight is 0.
Root Cause
The issue originates from the following calculation inside OnMouseWheel:
When ActualHeight is 0, the result becomes double.Infinity.
This value is later passed into the animation logic, which eventually converts it into a TimeSpan using TimeSpan.FromTicks. Since Infinity cannot be represented as a valid duration, a System.OverflowException is thrown.
Changes
Added a guard clause in CustomScrollViewerEx.cs to return early when:
Prevented invalid Infinity/NaN scroll offsets from reaching the animation pipeline.
Ensured the animation logic only receives finite values.
Testing
Added MouseWheelTest.cs to simulate a MouseWheelEvent on a CustomScrollViewerEx with zero dimensions.
Verified that:
without the fix, the scroll logic attempts to process an infinite offset;
with the fix, the method exits safely without triggering an exception.
I’m also open to feedback if you think there’s a cleaner or more robust approach for handling this scenario.
Summary by cubic
Prevents an OverflowException in
CustomScrollViewerExwhen mouse-wheel scrolling before layout by guarding on zeroActualHeight. Adds a unit test and minor cleanup with no behavior changes.Summary of changes
OnMouseWheelnow returns immediately whenActualHeight <= 0; guard moved to method start to avoid calculating wheel change on zero height. Minor code cleanup inCustomScrollViewerExwith no behavior change.MouseWheelTestthat invokesOnMouseWheelon an unrendered control; test project now referencesFlow.Launcher.Release Note
Fixes a crash when using the mouse wheel to scroll before the window finishes rendering.
Written for commit a372971. Summary will update on new commits.